Show skipped rows count after CSV import#3059
Conversation
📝 WalkthroughWalkthroughThis pull request modernizes the build output by replacing minified CSS stylesheets with expanded, source-map-enabled versions from SCSS compilation, refactors JavaScript modules from monolithic IIFEs to modular webpack bundles, and adds import progress tracking via a "skipped rows" query parameter in the PHP controller and admin AJAX handler. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Apr 6, 2026 3:16p.m. | Review ↗ |
| JavaScript | |
Apr 6, 2026 3:16p.m. | Review ↗ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.css (1)
120-121: Remove overridden background declaration for clarity.
background-positionis overridden by the nextbackgroundshorthand, so the first declaration is dead code.♻️ Suggested cleanup
.frm-dropdown-component select { @@ - background-position: center 5px; background: transparent url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='20' height='20' fill='none'%3E%3Cpath stroke='%2398A2B3' stroke-linecap='round' stroke-linejoin='round' stroke-width='1.5' d='M12.708 8.959 10 11.875 7.292 8.96'/%3E%3C/svg%3E") no-repeat right 8px top 50%;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/web-components/frm-dropdown-component/frm-dropdown-component.css` around lines 120 - 121, Remove the redundant background-position declaration in frm-dropdown-component.css: the standalone background-position property is immediately overridden by the following background shorthand, so delete the background-position: center 5px; line and keep the background shorthand (the declarations around the background and background-position in the frm-dropdown-component CSS block).js/src/web-components/frm-range-slider-component/frm-range-slider-component.css (1)
176-259: Consider centralizing shared design tokens instead of repeating per component.Duplicating the full
:roottoken block across component CSS increases payload and raises drift risk if one block changes later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/web-components/frm-range-slider-component/frm-range-slider-component.css` around lines 176 - 259, This file duplicates the full :root CSS variable block (seen on selectors :root, .frm-white-body, .frm_wrap); remove the large token block from frm-range-slider-component.css and instead import or reference a single shared design-token stylesheet (e.g., a global tokens.css loaded by the app) so components reuse the canonical variables; update any component-specific overrides to only include variables that truly differ from the global tokens and ensure selectors like .frm-range-slider-component only define local overrides, leaving :root, .frm-white-body, and .frm_wrap definitions in the centralized tokens file.js/src/web-components/frm-border-radius-component/frm-border-radius-component.css (1)
1-229: Split generated stylesheet churn from feature changes.This looks like build-output regeneration and is not clearly tied to the CSV skipped-rows feature. Please move this kind of generated CSS-only update to a dedicated PR to keep feature review focused and reduce merge-conflict noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css` around lines 1 - 229, This commit contains regenerated build output (the CSS file with :root variables and .frm-border-radius-component rules) that is unrelated to the CSV skipped-rows feature; revert or remove the changes to this generated stylesheet (.frm-border-radius-component CSS) from this PR and instead create a separate, focused PR that contains only the regenerated build artifact (or add it to your build-only branch). Specifically, drop the modified frm-border-radius-component.css (the rules under :root and .frm-border-radius-component selectors) from this diff, restore the previous tracked version in this branch, and submit the regenerated CSS in its own PR so the feature PR remains focused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/controllers/FrmEntriesController.php`:
- Line 648: Replace the plural-incorrect call to __( '(%d rows skipped)',
'formidable' ) with a plural-aware _n() usage: use _n( '(%d row skipped)', '(%d
rows skipped)', $skipped, 'formidable' ) and pass $skipped into sprintf so
$message .= ' ' . sprintf( _n(...), $skipped ); update the concatenation where
$message and $skipped are used (in FrmEntriesController around the code that
appends skipped row info) to ensure correct singular/plural translations.
In `@js/formidable_dashboard.js`:
- Around line 935-944: The AJAX welcome-banner dismissal request lacks CSRF
nonce protection; update the client and server to include and verify a nonce:
add the nonce field to the fetch body (use the same key used elsewhere, e.g.
'nonce') when calling the fetch in formidable_dashboard.js for the welcomeBanner
action, and in the server-side handler (FrmDashboardController::ajax_requests
and before calling add_welcome_closed_banner_user_id) call
check_ajax_referer('frm_ajax', 'nonce') to validate it and abort if invalid.
- Around line 401-402: The queuing logic in addToRequestQueue currently uses
lastPromise.then(task).catch(task), which can call task twice when task itself
rejects; change the chaining to use lastPromise.then(task, task) so the second
argument handles prior-rejection without re-invoking task on its own
failure—update the invocation in addToRequestQueue (and any similar use of
lastPromise.then(...).catch(...)) to use .then(task, task) referencing the
lastPromise variable and addToRequestQueue function.
- Around line 125-126: The beforeunload listener passes cleanupObservers with
wrong this (window), so cleanupObservers cannot access this.resizeObserver; bind
the method before registering it (e.g., use this.cleanupObservers =
this.cleanupObservers.bind(this) during initialization) or register a
bound/wrapper handler when calling window.addEventListener('beforeunload', ...).
Ensure the bound function reference is stored (so you can remove it later if
needed) and that cleanupObservers properly calls
this.resizeObserver.disconnect().
In
`@js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css`:
- Around line 169-170: The active-link selector currently only applies anchor
color inside .frm-style-tabs-wrapper, so active tab links in the generic
.frm-tabs-wrapper remain grey; update the selector for the .frm-active state
(used on .frm-tabs-navs ul li.frm-active) to also target the anchor element
outside .frm-style-tabs-wrapper (e.g., include .frm-tabs-navs ul li.frm-active a
or .frm-tabs-wrapper .frm-tabs-navs ul li.frm-active a) so the anchor inherits
the active color (refer to selectors .frm-tabs-navs, .frm-active,
.frm-style-tabs-wrapper, and .frm-tabs-wrapper when making the change).
---
Nitpick comments:
In
`@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css`:
- Around line 1-229: This commit contains regenerated build output (the CSS file
with :root variables and .frm-border-radius-component rules) that is unrelated
to the CSV skipped-rows feature; revert or remove the changes to this generated
stylesheet (.frm-border-radius-component CSS) from this PR and instead create a
separate, focused PR that contains only the regenerated build artifact (or add
it to your build-only branch). Specifically, drop the modified
frm-border-radius-component.css (the rules under :root and
.frm-border-radius-component selectors) from this diff, restore the previous
tracked version in this branch, and submit the regenerated CSS in its own PR so
the feature PR remains focused.
In `@js/src/web-components/frm-dropdown-component/frm-dropdown-component.css`:
- Around line 120-121: Remove the redundant background-position declaration in
frm-dropdown-component.css: the standalone background-position property is
immediately overridden by the following background shorthand, so delete the
background-position: center 5px; line and keep the background shorthand (the
declarations around the background and background-position in the
frm-dropdown-component CSS block).
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.css`:
- Around line 176-259: This file duplicates the full :root CSS variable block
(seen on selectors :root, .frm-white-body, .frm_wrap); remove the large token
block from frm-range-slider-component.css and instead import or reference a
single shared design-token stylesheet (e.g., a global tokens.css loaded by the
app) so components reuse the canonical variables; update any component-specific
overrides to only include variables that truly differ from the global tokens and
ensure selectors like .frm-range-slider-component only define local overrides,
leaving :root, .frm-white-body, and .frm_wrap definitions in the centralized
tokens file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20f26aaa-b640-485e-adc6-b55e27ad3dbc
📒 Files selected for processing (25)
classes/controllers/FrmEntriesController.phpcss/admin/frm-settings-components.csscss/admin/welcome-tour.csscss/font_icons.csscss/frm_admin.csscss/frm_testing_mode.cssjs/addons-page.jsjs/form-templates.jsjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable_admin.jsjs/formidable_blocks.jsjs/formidable_dashboard.jsjs/formidable_overlay.jsjs/formidable_styles.jsjs/frm_testing_mode.jsjs/onboarding-wizard.jsjs/src/admin/admin.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.cssjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.cssjs/src/web-components/frm-dropdown-component/frm-dropdown-component.cssjs/src/web-components/frm-range-slider-component/frm-range-slider-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-typography-component/frm-typography-component.cssjs/welcome-tour.js
| $message = __( 'Your import is complete', 'formidable' ); | ||
| $skipped = isset( $_GET['skipped'] ) ? intval( $_GET['skipped'] ) : 0; | ||
| if ( $skipped > 0 ) { | ||
| $message .= ' ' . sprintf( __( '(%d rows skipped)', 'formidable' ), $skipped ); |
There was a problem hiding this comment.
Use plural-aware translation for skipped row count.
__( '(%d rows skipped)', 'formidable' ) produces incorrect grammar for 1 and is harder to localize. Please switch to _n() with the count.
💡 Suggested fix
- $message .= ' ' . sprintf( __( '(%d rows skipped)', 'formidable' ), $skipped );
+ $message .= ' ' . sprintf(
+ _n( '(%d row skipped)', '(%d rows skipped)', $skipped, 'formidable' ),
+ $skipped
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $message .= ' ' . sprintf( __( '(%d rows skipped)', 'formidable' ), $skipped ); | |
| $message .= ' ' . sprintf( | |
| _n( '(%d row skipped)', '(%d rows skipped)', $skipped, 'formidable' ), | |
| $skipped | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/controllers/FrmEntriesController.php` at line 648, Replace the
plural-incorrect call to __( '(%d rows skipped)', 'formidable' ) with a
plural-aware _n() usage: use _n( '(%d row skipped)', '(%d rows skipped)',
$skipped, 'formidable' ) and pass $skipped into sprintf so $message .= ' ' .
sprintf( _n(...), $skipped ); update the concatenation where $message and
$skipped are used (in FrmEntriesController around the code that appends skipped
row info) to ensure correct singular/plural translations.
| // Cleanup observers when page unloads to prevent memory leaks | ||
| window.addEventListener('beforeunload', this.cleanupObservers); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(formidable|dashboard|class-tabs)" | head -20Repository: Strategy11/formidable-forms
Length of output: 941
🏁 Script executed:
find . -type f -name "*.js" | grep -E "(dashboard|tabs|navigator)" | head -20Repository: Strategy11/formidable-forms
Length of output: 306
🏁 Script executed:
head -130 ./js/formidable_dashboard.js | tail -10Repository: Strategy11/formidable-forms
Length of output: 400
🏁 Script executed:
grep -n "cleanupObservers" ./js/formidable_dashboard.js | head -10Repository: Strategy11/formidable-forms
Length of output: 222
🏁 Script executed:
sed -n '179,195p' ./js/formidable_dashboard.jsRepository: Strategy11/formidable-forms
Length of output: 601
🏁 Script executed:
cat -n ./js/src/components/class-tabs-navigator.js | head -150Repository: Strategy11/formidable-forms
Length of output: 5174
🏁 Script executed:
head -5 ./js/formidable_dashboard.jsRepository: Strategy11/formidable-forms
Length of output: 218
🏁 Script executed:
git log --oneline --all | head -1Repository: Strategy11/formidable-forms
Length of output: 121
Bind cleanupObservers before registering it as a listener.
The method is passed as a bare reference to beforeunload, which invokes it with this === window. This prevents this.resizeObserver from being properly disconnected, causing memory leaks.
🧩 Minimal fix
- window.addEventListener( 'beforeunload', this.cleanupObservers );
+ window.addEventListener( 'beforeunload', this.cleanupObservers.bind( this ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener('beforeunload', this.cleanupObservers); | |
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener( 'beforeunload', this.cleanupObservers.bind( this ) ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 125 - 126, The beforeunload listener
passes cleanupObservers with wrong this (window), so cleanupObservers cannot
access this.resizeObserver; bind the method before registering it (e.g., use
this.cleanupObservers = this.cleanupObservers.bind(this) during initialization)
or register a bound/wrapper handler when calling
window.addEventListener('beforeunload', ...). Ensure the bound function
reference is stored (so you can remove it later if needed) and that
cleanupObservers properly calls this.resizeObserver.disconnect().
| var addToRequestQueue = function addToRequestQueue(task) { | ||
| return lastPromise = lastPromise.then(task).catch(task); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "async\.js" | head -20Repository: Strategy11/formidable-forms
Length of output: 97
🏁 Script executed:
git ls-files | grep -E "(async|queue)" | head -20Repository: Strategy11/formidable-forms
Length of output: 97
🏁 Script executed:
cat -n js/src/core/utils/async.jsRepository: Strategy11/formidable-forms
Length of output: 545
🏁 Script executed:
head -20 js/formidable_dashboard.jsRepository: Strategy11/formidable-forms
Length of output: 1918
🏁 Script executed:
grep -n "addToRequestQueue" js/formidable_dashboard.js | head -5Repository: Strategy11/formidable-forms
Length of output: 348
🏁 Script executed:
sed -n '401,402p' js/formidable_dashboard.jsRepository: Strategy11/formidable-forms
Length of output: 188
Don't rerun failed queued tasks.
The current implementation in js/src/core/utils/async.js uses .catch(task) which catches rejections from both the previous promise and from task itself. When task fails, the rejection handler re-executes it, allowing a failed write to run twice.
Use .then(task, task) instead. The second parameter to .then() is the rejection handler that catches previous promise failures without re-executing task if it fails.
🔁 Queue each task at most once
var addToRequestQueue = function addToRequestQueue(task) {
- return lastPromise = lastPromise.then(task).catch(task);
+ return lastPromise = lastPromise.then(task, task);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var addToRequestQueue = function addToRequestQueue(task) { | |
| return lastPromise = lastPromise.then(task).catch(task); | |
| var addToRequestQueue = function addToRequestQueue(task) { | |
| return lastPromise = lastPromise.then(task, task); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 401 - 402, The queuing logic in
addToRequestQueue currently uses lastPromise.then(task).catch(task), which can
call task twice when task itself rejects; change the chaining to use
lastPromise.then(task, task) so the second argument handles prior-rejection
without re-invoking task on its own failure—update the invocation in
addToRequestQueue (and any similar use of lastPromise.then(...).catch(...)) to
use .then(task, task) referencing the lastPromise variable and addToRequestQueue
function.
| return fetch(window.ajaxurl, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded' | ||
| }, | ||
| body: new URLSearchParams({ | ||
| action: this.options.ajax.action, | ||
| dashboard_action: this.options.ajax.dashboardActions.welcomeBanner, | ||
| banner_has_closed: 1 | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the welcome-banner AJAX path for nonce handling.
rg -n -C3 "welcome-banner-has-closed|check_ajax_referer|dashboard_ajax_action" classesRepository: Strategy11/formidable-forms
Length of output: 29328
🏁 Script executed:
# Get the ajax_requests method implementation in FrmDashboardController
rg -n -A 40 "public function ajax_requests" classes/controllers/FrmDashboardController.phpRepository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Look for nonce verification in FrmDashboardController
rg -n "check_ajax_referer\|wp_verify_nonce" classes/controllers/FrmDashboardController.phpRepository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Check the JavaScript file around lines 935-944
sed -n '920,960p' js/formidable_dashboard.js | cat -nRepository: Strategy11/formidable-forms
Length of output: 1488
🏁 Script executed:
fd -i "FrmDashboardController" --type fRepository: Strategy11/formidable-forms
Length of output: 117
🏁 Script executed:
# Also check what files are in the controllers directory
git ls-files classes/controllers/ | grep -i dashboardRepository: Strategy11/formidable-forms
Length of output: 117
🏁 Script executed:
# Get file size and read it
wc -l classes/controllers/FrmDashboardController.phpRepository: Strategy11/formidable-forms
Length of output: 121
🏁 Script executed:
# Read the ajax_requests method and surrounding code
sed -n '310,360p' classes/controllers/FrmDashboardController.php | cat -nRepository: Strategy11/formidable-forms
Length of output: 1916
🏁 Script executed:
# Search for add_welcome_closed_banner_user_id method
rg -n "add_welcome_closed_banner_user_id" classes/controllers/FrmDashboardController.php -A 10Repository: Strategy11/formidable-forms
Length of output: 790
🏁 Script executed:
# Check the hook registration in FrmHooksController
rg -n -B 2 -A 2 "dashboard_ajax_action" classes/controllers/FrmHooksController.phpRepository: Strategy11/formidable-forms
Length of output: 232
Add nonce protection to the welcome-banner dismissal AJAX request.
The POST request at lines 935-944 mutates dashboard state without CSRF protection. The FrmDashboardController::ajax_requests() method (line 321) receives the welcome-banner-has-closed action and calls add_welcome_closed_banner_user_id() (line 554), which updates options, but neither function verifies a nonce. This leaves the endpoint vulnerable to CSRF attacks from any logged-in user.
Send a nonce in the request body (matching the pattern used elsewhere in the codebase with check_ajax_referer('frm_ajax', 'nonce')) and validate it server-side before updating options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 935 - 944, The AJAX welcome-banner
dismissal request lacks CSRF nonce protection; update the client and server to
include and verify a nonce: add the nonce field to the fetch body (use the same
key used elsewhere, e.g. 'nonce') when calling the fetch in
formidable_dashboard.js for the welcomeBanner action, and in the server-side
handler (FrmDashboardController::ajax_requests and before calling
add_welcome_closed_banner_user_id) call check_ajax_referer('frm_ajax', 'nonce')
to validate it and abort if invalid.
| .frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a { | ||
| color: var(--grey-900); |
There was a problem hiding this comment.
Fix the active-link selector scope.
Lines 153-159 set color directly on li a, but the active override here still limits anchors to .frm-style-tabs-wrapper. In the generic .frm-tabs-wrapper cases this stylesheet defines everywhere else, the active tab link keeps the inactive grey.
🎯 Suggested selector fix
-.frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a {
+.frm-tabs-navs ul li.frm-active,
+.frm-tabs-navs ul li.frm-active a {
color: var(--grey-900);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a { | |
| color: var(--grey-900); | |
| .frm-tabs-navs ul li.frm-active, | |
| .frm-tabs-navs ul li.frm-active a { | |
| color: var(--grey-900); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css`
around lines 169 - 170, The active-link selector currently only applies anchor
color inside .frm-style-tabs-wrapper, so active tab links in the generic
.frm-tabs-wrapper remain grey; update the selector for the .frm-active state
(used on .frm-tabs-navs ul li.frm-active) to also target the anchor element
outside .frm-style-tabs-wrapper (e.g., include .frm-tabs-navs ul li.frm-active a
or .frm-tabs-wrapper .frm-tabs-navs ul li.frm-active a) so the anchor inherits
the active color (refer to selectors .frm-tabs-navs, .frm-active,
.frm-style-tabs-wrapper, and .frm-tabs-wrapper when making the change).
1 participant
Related PR: https://github.com/Strategy11/formidable-pro/pull/4287
Summary by CodeRabbit
Release Notes
New Features
Refactor